Add `Clash.Class.NumConvert` #2915

martijnbastiaan · 2025-03-30T19:04:22Z

Utilities for safely converting between various Clash number types.

To discuss / investigate (but maybe not for this PR?)

The instances add 1 <= n constraints to every result where the result is Index n
- This makes sense from a perspective of: Index 0 has no inhabitants, so we cannot create it. On the other hand, we can of course just call errorX "foo". I'm not sure whether I would say this violates the "guaranteed to succeed" rule.
The instances add 1 <= n constraints to every argument where the argument is Index n
- This is similar to the first point, but differs in that we shove the responsibility for creating these values to the caller.
convert and maybeConvert will always return XException if given an XException.
- Does this make sense for converting BitVector 0 to Index/Unsigned/Signed? The HDL will happily do it, why wouldn't Clash simulation? Similarly, any BitVector n could be repacked to be partially undefined if converted to another BitVector m where m > n.

I'd push for kicking these questions to after this PR, because:

We can always relax constraints (but not introduce them)
We can always relax XException behavior (but not introduce it).
Having a safe way of converting between Clash types is way more important than dropping a minor constraint.
It's a fairly delicate thing I don't think we have a proper/consistent view on yet across clash-prelude.

Still TODO:

Write a changelog entry (see changelog/README.md)
Check copyright notices are up to date in edited files
Decide whether to call it Clash.Class.Convert or Clash.Class.Convertible. The latter has some buy-in (?) from the community, given that covertible already exists. On the other hand, this might make users think it is convertible, but it isn't. NumConvert!

DigitalBrains1 · 2025-03-30T20:26:48Z

I'd vote against naming it Convertible, since I think GHC will by default not qualify the class name in an error message of the form No instance for ‘Convertible A B’, which might be very misleading.

_{(I really want to make the silly pun No instance for ‘Convertible Coupé Roadster’ but it seems confusing... X-D)}

clash-prelude/src/Clash/Class/Convert.hs

clash-prelude/src/Clash/Class/Convert/Internal/Convert.hs

clash-prelude/src/Clash/Class/Convert/Internal/MaybeConvert.hs

kleinreact

LGTM overall.

I'm going to accept this, because I cannot see anything really controversial in the sense that it would be hard to fix it later on.

Regarding the Convert vs Convertible question: I don't like either name, as they both suggest much freedom in terms of which types could be converted with the classes. Your documentation clearly restricts the setting to number types, which is in contrast to Convertible for example, which is also open for conversion between custom non-number types. So maybe we should keep the users of that package happy by not producing any name clash at this point.

A name like changeNumType would make it more clear to me what the purpose of this function is. Convert, Convertible, and convert are just super generic names!

I still find it strange to consider BitVector a number type. If we are going to have a discussion about this at some point, then I'd suggest to keep it out here for the time being. Adding it later is easy and the user still can define the instance locally, if necessary.

I don't like the fact that we need to have two classes in the end, but I get the point that the need for individual selection of constraints per instance introduces that requirement.

clash-prelude/src/Clash/Class/Convert/Internal/Convert.hs

clash-prelude/src/Clash/Class/Convert/Internal/MaybeConvert.hs

martijnbastiaan · 2025-04-10T08:18:02Z

Thanks for the comments @kleinreact, I'll get to them, though I'm fairly time constrained at the moment :(.

I think your comment about the name convert makes a lot of sense. Maybe convertNum? You're right that the class really constrains it to just number types. (Maybe that is an issue?)

DigitalBrains1 · 2025-04-10T10:34:35Z

Somehow I feel NumConvert feels more natural than ConvertNum for the class name... But not so for the function name.

Also, we could rewrite the laws such that the class can also be used for non-numbers. I think that would work?

DigitalBrains1

I also feel Convert is too generic a name and squats too much, except if it is only to be used qualified. I'm personally leaning towards renaming it NumConvert, numConvert, MaybeNumConvert and maybeNumConvert as the names used, and exporting them from Prelude. Note that you currently to not export them from Prelude, I'd like that to be added.

Something I noticed is that for Convert, you seem to put a bang pattern only on the instances that need one to preserve the XException, yet on MaybeConvert you just bang almost everything. Was this intended? I'm not against it, I'm just wondering.

clash-prelude/src/Clash/Class/Convert/Internal/Convert.hs

DigitalBrains1 · 2025-04-24T15:28:15Z

clash-prelude/src/Clash/Class/Convert/Internal/Convert.hs

+
+Instances should make sure their constraints are as \"tight\" as possible. I.e.,
+if an instance exist, but the constraints cannot be satisfied, then
+'Clash.Class.Convert.convertMaybe' should return 'Nothing' for one or more values in


Suggested change

'Clash.Class.Convert.convertMaybe' should return 'Nothing' for one or more values in

'maybeConvert' should return 'Nothing' for one or more values in

This is a dead link.

The qualification is not necessary; the function is documented in this module. If it's unqualified, our Haddock CI check will notice the dead link, unlike in the qualified situation.

But regarding this whole paragraph: I found it hard to understand because I misinterpreted what it means for an instance to exist. Because to me, if the constraints are not satisfied, an instance does not exist. So I'd say that there is no instance for Convert (Unsigned 8) (Unsigned 4), whereas the language in this paragraph says the instance exists, but its constraints can't be satisfied. I hope we can rephrase it to avoid this pitfall.

What do you think of:

Instances should make sure their constraints are as \"tight\" as possible. I.e., if an instance's constraints cannot be satisfied, then for the same types 'Clash.Class.NumConvert.maybeConvert' should return 'Nothing' for one or more values in the domain of the source type @a@:

I find it quite hard to move away from my mental model of "instance resolver has found a match == instance exists", so I'm having trouble really rewriting this.

clash-prelude/src/Clash/Class/Convert/Internal/Convert.hs

clash-prelude/src/Clash/Class/Convert/Internal/MaybeConvert.hs

clash-prelude/tests/Clash/Tests/Convert.hs

clash-prelude/tests/Clash/Tests/MaybeConvert.hs

clash-prelude/src/Clash/Class/Convert/Internal/Convert.hs

martijnbastiaan · 2025-05-17T13:00:38Z

@DigitalBrains1 For some reason GitHub doesn't let me reply to your comment on the word total, so I'll reply here: totally fair criticism! I think lossless is the word I want to use. (After rephrasing the sentence a bit.)

DigitalBrains1 · 2025-05-17T13:39:42Z

For some reason GitHub doesn't let me reply to your comment on the word total

Yeah, that's a confusing formatting issue. As part of a review, you can reply to an earlier review comment. It then shows it in the original place where you can reply, and also as part of the new review. If you click on the date in the header of the latter (3 weeks ago it currently says), you can go to the former and comment... why they did this, I don't know.

I think lossless is fair, although you could argue that bitCoerce @(Unsigned 8) @(Signed 8) 200 is in fact lossless even though the result is -56. But when put into context, lossless is in fact a proper description that people should be able to understand.

[edit]

totally fair criticism!

Wait, was that a pun? Did I miss a pun on my first read? X-D
Nice one!
[/edit]

martijnbastiaan · 2025-05-17T14:30:38Z

Me? Puns? I wouldn't dare!

martijnbastiaan · 2025-05-18T18:59:54Z

I've done a couple of things (now hidden in resolved conversations):

I've renamed the class
I've touched up the documentation pointed out by the reviews
I've fixed a few issues with the test bench
I've added instances for Bit

I think I got to all the comments. ~~Edit: export from Prelude TBD.~~

martijnbastiaan · 2025-05-19T07:17:32Z

Something I noticed is that for Convert, you seem to put a bang pattern only on the instances that need one to preserve the XException, yet on MaybeConvert you just bang almost everything. Was this intended? I'm not against it, I'm just wondering.

I mostly followed the error messages brought up by the tests. TBF, it is a bit too much for me to do precisely so I might have been over eager..

Utilities for safely converting between various Clash number types

This comment was marked as resolved.

Sign in to view

DigitalBrains1 self-requested a review March 30, 2025 19:19

This comment was marked as resolved.

Sign in to view

martijnbastiaan force-pushed the martijn/add-clash-class-convert branch 2 times, most recently from d8f851c to 25c7757 Compare April 1, 2025 20:43

martijnbastiaan requested a review from kleinreact April 2, 2025 07:10

martijnbastiaan added the needs-review label Apr 2, 2025

martijnbastiaan commented Apr 2, 2025

View reviewed changes

clash-prelude/src/Clash/Class/Convert.hs Outdated Show resolved Hide resolved

clash-prelude/src/Clash/Class/Convert/Internal/Convert.hs Outdated Show resolved Hide resolved

clash-prelude/src/Clash/Class/Convert/Internal/MaybeConvert.hs Outdated Show resolved Hide resolved

kleinreact approved these changes Apr 7, 2025

View reviewed changes

DigitalBrains1 requested changes May 13, 2025

View reviewed changes

DigitalBrains1 reviewed May 14, 2025

View reviewed changes

clash-prelude/src/Clash/Class/Convert/Internal/Convert.hs Outdated Show resolved Hide resolved

martijnbastiaan force-pushed the martijn/add-clash-class-convert branch 3 times, most recently from 0ba7f07 to 73d2424 Compare May 18, 2025 18:11

martijnbastiaan removed the needs-review label May 18, 2025

martijnbastiaan changed the title ~~Add Clash.Class.Convert~~ Add Clash.Class.NumConvert May 18, 2025

martijnbastiaan force-pushed the martijn/add-clash-class-convert branch 4 times, most recently from 13064f3 to b805b7b Compare May 18, 2025 18:49

martijnbastiaan requested a review from DigitalBrains1 May 18, 2025 18:49

martijnbastiaan force-pushed the martijn/add-clash-class-convert branch from b805b7b to 42406b1 Compare May 18, 2025 18:54

martijnbastiaan force-pushed the martijn/add-clash-class-convert branch 2 times, most recently from 9a88bca to d74051a Compare May 19, 2025 07:25

Add Clash.Class.NumConvert

530541f

Utilities for safely converting between various Clash number types

martijnbastiaan force-pushed the martijn/add-clash-class-convert branch from d74051a to 530541f Compare May 19, 2025 07:47

	'Clash.Class.Convert.convertMaybe' should return 'Nothing' for one or more values in
	'maybeConvert' should return 'Nothing' for one or more values in

Add Clash.Class.NumConvert #2915

Are you sure you want to change the base?

Add Clash.Class.NumConvert #2915

Uh oh!

Conversation

martijnbastiaan commented Mar 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To discuss / investigate (but maybe not for this PR?)

Still TODO:

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

DigitalBrains1 commented Mar 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kleinreact left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

martijnbastiaan commented Apr 10, 2025

Uh oh!

DigitalBrains1 commented Apr 10, 2025

Uh oh!

DigitalBrains1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DigitalBrains1 Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

martijnbastiaan May 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

martijnbastiaan commented May 17, 2025

Uh oh!

DigitalBrains1 commented May 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martijnbastiaan commented May 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martijnbastiaan commented May 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martijnbastiaan commented May 19, 2025

Uh oh!

Uh oh!

Add `Clash.Class.NumConvert` #2915

Add `Clash.Class.NumConvert` #2915

martijnbastiaan commented Mar 30, 2025 •

edited

Loading

DigitalBrains1 commented Mar 30, 2025 •

edited

Loading

DigitalBrains1 commented May 17, 2025 •

edited

Loading

martijnbastiaan commented May 17, 2025 •

edited

Loading

martijnbastiaan commented May 18, 2025 •

edited

Loading